-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ctr: use BlockEncrypt
instead of BlockEncryptMut
#14
Conversation
As far as I can tell this is just a mistake: the only state in CTR mode is the counter, and there is no reason to mandate some sort of stateful block cipher as part of the bounds. A `BlockCipherMut` bound is a significant impediment to upgrading the AEAD crates, which all impl `AeadInPlace` instead of `AeadMutInPlace`, store block cipher instances (which avoids re-expanding keys), and need to be able to share non-mutable references to those block cipher instances.
@newpavlov I notice that all of the crates in this repo seem to use |
No, |
This makes it impossible to use the I'm aware of the embedded applications: I was the one who originally added |
|
Here's a contrived example (minimized from the pub struct MyAead<C> {
cipher: C
}
impl<C> MyAead<C>
where
C: BlockCipher + BlockEncrypt + BlockSizeUser<BlockSize = U16>,
{
pub fn init_ctr(&self) -> Ctr32BE<&C> {
Ctr32BE::inner_iv_init(&self.cipher, &Default::default())
}
} This results in the following error:
If I try
If I try both:
If I try adding bounds to the reference type: pub struct MyAead<C> {
cipher: C
}
impl<C> MyAead<C>
where
C: BlockCipher + BlockEncrypt + BlockSizeUser<BlockSize = U16>,
{
pub fn init_ctr<'a>(&'a self) -> Ctr32BE<&'a C>
where
&'a C: BlockCipher + BlockEncryptMut + BlockSizeUser<BlockSize = U16>,
{
Ctr32BE::inner_iv_init(&self.cipher, &Default::default())
}
} then...
Even if there were a way to make this work, if I can't figure out how to do it, no one else will. Stateless block ciphers are far and away going to be the most common use case, so I don't think it makes sense to attempt to gloss over mutability in ways that will cause a confusing user experience. |
Ah, I see. The problem you are hitting here is outlined in this comment. I've chose to implement You can work around it by manually creating a core CTR instance first: pub fn init_ctr(&self) -> Ctr32BE<&C> {
let core = CtrCore::inner_iv_init(&self.cipher, &Default::default());
Ctr32BE::from_core(core)
} I agree that the current situation is not ideal, but I do not think that we should artificially limit block mode capabilities to work around language limitations. |
BTW since in AEAD crates we allow processing of only fully available messages, you do not need the |
I think the proper and idiomatic Rust solution is to provide
Speaking as the person who added With the forthcoming stabilization of GATs that will soon be possible in stable Rust. But if the trait moves from just
I'll try to make that work. It's a bit more complex than that as both AES-GCM and ChaCha20Poly1305 both require extracting the beginning of the keystream for a MAC parameter (a mask for AES-GCM, and the Poly1305 key for ChaCha20Poly1305). But I think it can be expressed in terms of |
Sidebar: I'm a bit confused why Still might be able to make things work by using |
I disagree. It will be a completely unnecessary multiplication of exposed types. Note that moving from You may remember my negative opinion about Rust's approach to async, so I can not say much about potential extension of our traits to the async environment.
"Partial" here means that we use only a part of generated keystream last block and discard remaining bytes. Similarly to the padding methods, the idea is that a stream cipher instance should not be used after calling this method. |
I confirmed that this PR would fix the issues I was encountering adapting the |
If there's one thing we should be optimizing for above all else, it should be clarity. Relying on chained blanket impls which allow invoking traits on reference types in order to allow you to pass an immutable reference to something that's expecting a mutable reference is not clear whatsoever. It is extremely confusing. I was confused, and even after understanding how it's supposed to work I was still puzzling over the documentation trying to check which paths through the blanket impls are possible. It's some of the most confusing Rust code I've ever encountered. If I find it confusing, people with no experience with this codebase at all are going to find it extremely confusing. And as you admitted, the abstraction leaks in several places:
This is not a good abstraction. It's a leaky abstraction that's harming clarity and usability, and those matter a hell of a lot more than eliminating code duplication, especially in cryptography.
|
Introducing a bunch of identical, but not quite, types will not help with clarity. Let's limit this discussion strictly to encrypt/decrypt traits and their application to the block mode crates. If you want to discuss API of the initialization traits and the issue with the stream wrapper, I think it's better to do it in a separate traits issue.
I think we have a different understanding of what a leaky abstraction is. To me it sounds like arguing that our use of Same applies to the lack of mutually exclusive traits. If we had an ability to specify that a type can not implement both We can improve "clarity" by making methods inherent or by removing the generic wrappers and instead using macros to generate code. But in my opinion it would be significant a step back. Currently, the generic wrappers not only reduce code duplication, but also encapsulate In abstract, I believe that after you get familiar with the current hierarchy of traits, it is a good and somewhat elegant approach (though I can not call myself impartial in this regard). Yes, we are hitting certain language limitations (not only mutually exclusive traits, but also lack of rank-2 closures), which does not make things easier. But I think we should push for language improvement instead of dumbing down current code, reducing its flexibility and ballooning amount of boilerplate in the process. I admit that
What? All block modes work through them. Maybe you meant that no one currently uses modes with a type which implements only I agree that having Assuming that the embedded use case will be properly covered by hypothetical async traits, I am open to potentially changing it in |
This sounds like a good solution to me which clearly reflects the usage patterns. Thanks! Will close this PR then. |
Renames the following traits: - `BlockEncrypt` => `BlockCipherEncrypt` - `BlockDecrypt` => `BlockCipherDecrypt` - `BlockEncryptMut` => `BlockModeEncrypt` - `BlockDecryptMut` => `BlockModeDecrypt` As originally suggested in this comment: RustCrypto/block-modes#14 (comment) This better reflects how these traits are actually used, i.e. `BlockCipher*` is used by ciphers, and `BlockMode*` is used by their modes of operation.
Renames the following traits: - `BlockEncrypt` => `BlockCipherEncrypt` - `BlockDecrypt` => `BlockCipherDecrypt` - `BlockEncryptMut` => `BlockModeEncrypt` - `BlockDecryptMut` => `BlockModeDecrypt` As originally suggested in this comment: RustCrypto/block-modes#14 (comment) This better reflects how these traits are actually used, i.e. `BlockCipher*` is used by ciphers, and `BlockMode*` is used by their modes of operation.
Renames the following traits: - `BlockEncrypt` => `BlockCipherEncrypt` - `BlockDecrypt` => `BlockCipherDecrypt` - `BlockEncryptMut` => `BlockModeEncrypt` - `BlockDecryptMut` => `BlockModeDecrypt` As originally suggested in this comment: RustCrypto/block-modes#14 (comment) This better reflects how these traits are actually used, i.e. `BlockCipher*` is used by ciphers, and `BlockMode*` is used by their modes of operation. Also removes the `*_mut` suffixes from `BlockMode*` methods.
As far as I can tell this is just a mistake: the only state in CTR mode is the counter, and there is no reason to mandate some sort of stateful block cipher as part of the bounds.
A
BlockCipherMut
bound is a significant impediment to upgrading the AEAD crates, which all implAeadInPlace
instead ofAeadMutInPlace
, store block cipher instances (which avoids re-expanding keys), and need to be able to share immutable references to those block cipher instances.